Skip to content

Make StorageCredentialCache safe for multi-realm usage #2021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 13, 2025
Merged

Make StorageCredentialCache safe for multi-realm usage #2021

merged 1 commit into from
Jul 13, 2025
+83 −87

Conversation

XN137
Copy link
Contributor

@XN137 XN137 commented Jul 10, 2025 •
edited
Loading

Injecting the request-scoped RealmContext into the application-scoped StorageCredentialCache makes things unnecessarily complicated.
Similarly StorageCredentialCacheKey having a @Nullable callContext makes it more difficult to reason about.

Instead we can determine all realm-specific values at the time of insertion (from the PolarisCallContext param of getOrGenerateSubScopeCreds).

@github-project-automation github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Jul 10, 2025
@XN137 XN137 changed the title Make StorageCredentialCache safe for mutli-realm usage Make StorageCredentialCache safe for multi-realm usage Jul 10, 2025
@XN137 XN137 marked this pull request as ready for review July 10, 2025 07:43
@XN137 XN137 mentioned this pull request Jul 10, 2025
Draft
snazy
snazy reviewed Jul 10, 2025
View reviewed changes
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks good to me, and it doesn't change the semantics.

While reviewing, I noticed that the cache key can become quite large, depending on the paths and storage config. Not sure whether there's a better way.

The thing that's not fully clear to me is how long a cached storage-credentials-entry is usable. I see that the cache-entry expiration is calculated in org.apache.polaris.core.storage.cache.StorageCredentialCacheEntry#getExpirationTime, but that returns the expiration time of the cached credentials. I wonder how long a client can actually use such credentials?

...ime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java Outdated
@@ -465,7 +465,7 @@ public Supplier<TransactionalPersistence> getOrCreateSessionSupplier(

@Override
public StorageCredentialCache getOrCreateStorageCredentialCache(RealmContext realmContext) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If RealmContext is no longer needed, let's remove or at least deprecate-for-removal this function and add one that takes no parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have a followup PR-2022 for dealing with this method, generally speaking since StorageCredentialCache is application-scoped and multi-realm then this method is no longer needed at all afaict.

its only getting called from a single place RealmEntityManagerFactory so idk if deprecation would achieve much but i can add it.

polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
* The callContext is passed to be used to fetch subscoped creds, but is not used to hash/equals
* as part of the cache key.
*/
private @Nullable PolarisCallContext callContext;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the removal of this field is the key of this PR?

It totally makes sense to not keep per-request state around.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but also removal of the realmContext field is important as well, as it is request scope but was being used inside the expiry logic of the cache.

...is-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheEntry.java
@@ -32,12 +32,19 @@ public class StorageCredentialCacheEntry {
public final EnumMap<StorageAccessProperty, String> credsMap;

private final ScopedCredentialsResult scopedCredentialsResult;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: this field is unused

dimas-b
dimas-b previously approved these changes Jul 10, 2025
View reviewed changes
Copy link
Contributor

@dimas-b dimas-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks, @XN137 !

@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 10, 2025
@XN137 XN137 force-pushed the StorageCredentialCache-multi-realm branch from c4fb8f5 to 2ad970b Compare July 11, 2025 07:38
@XN137
Copy link
Contributor Author

XN137 commented Jul 11, 2025

rebased after conflict with 19f44d8

runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/IcebergCatalogTest.java
runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolarisGenericTableCatalogTest.java
runtime/service/src/test/java/org/apache/polaris/service/quarkus/catalog/PolicyCatalogTest.java

snazy
snazy approved these changes Jul 11, 2025
View reviewed changes
Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@XN137 XN137 mentioned this pull request Jul 11, 2025
Draft
@eric-maynard eric-maynard merged commit 6ddd148 into apache:main Jul 13, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jul 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snazy snazy snazy approved these changes

@dimas-b dimas-b dimas-b approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@XN137 @snazy @dimas-b @eric-maynard